Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MPDX-7987 Merge Contacts #959

Merged
merged 14 commits into from
Jun 24, 2024
Merged

MPDX-7987 Merge Contacts #959

merged 14 commits into from
Jun 24, 2024

Conversation

caleballdrin
Copy link
Contributor

@caleballdrin caleballdrin commented Jun 18, 2024 •
edited
Loading

Description

https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-7987#

  • Make confirm buttons work
  • Make the green border thicker
  • Fix date
  • Use proxy API

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@caleballdrin caleballdrin requested a review from dr-bizz June 18, 2024 15:57
@caleballdrin caleballdrin self-assigned this Jun 18, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 18, 2024 •
edited
Loading

Bundle sizes [mpdx-react]

Compared against f64dd8a

Route Size (gzipped) Diff
/accountLists/[accountListId]/tools/mergeContacts/[[...contactId]] 114.43 KB +6.39 KB

@caleballdrin caleballdrin force-pushed the 7987-merge-contacts branch from bff4816 to 560f331 Compare June 18, 2024 16:00
dr-bizz
dr-bizz requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. I lef a few comments nothing crazy.

I did see that the sorting of contacts is different from the React app va to the old Angular app. I would think they need to be the same to not confuse people.

I'm looking at TTM | John Doe | Lois Lane on the apps account

pages/api/Schema/MergeContacts/mergeContacts.graphql Outdated Show resolved Hide resolved
src/components/Tool/MergeContacts/Contact.tsx Outdated Show resolved Hide resolved
src/components/Tool/MergeContacts/Contact.tsx Outdated Show resolved Hide resolved
src/components/Tool/MergeContacts/GetContactDuplicates.graphql Outdated Show resolved Hide resolved
src/components/Tool/MergeContacts/MergeContacts.test.tsx Show resolved Hide resolved
@dr-bizz

This comment was marked as resolved.

Sign in to view
@caleballdrin

This comment was marked as resolved.

Sign in to view
@dr-bizz

This comment was marked as resolved.

Sign in to view
@caleballdrin caleballdrin force-pushed the 7987-merge-contacts branch from b7e6d05 to 7994eed Compare June 19, 2024 05:14
@caleballdrin
Copy link
Contributor Author

caleballdrin commented Jun 19, 2024 •
edited
Loading

This is looking great. I lef a few comments nothing crazy.

I did see that the sorting of contacts is different from the React app va to the old Angular app. I would think they need to be the same to not confuse people.

I'm looking at TTM | John Doe | Lois Lane on the apps account

The angular app gets at the last _ rows rather than the first rows. I'm going to have the React app start at the beginning so it is in alphabetical order. Does that sound good?

@caleballdrin caleballdrin requested a review from dr-bizz June 19, 2024 05:19
@caleballdrin caleballdrin added the Preview Environment Add this label to create an Amplify Preview label Jun 19, 2024
@github-actions GitHub Actions
Copy link
Contributor

Preview branch generated at https://7987-merge-contacts.d3dytjb8adxkk5.amplifyapp.com

@dr-bizz
Copy link
Contributor

dr-bizz commented Jun 19, 2024

This is looking great. I lef a few comments nothing crazy.
I did see that the sorting of contacts is different from the React app va to the old Angular app. I would think they need to be the same to not confuse people.
I'm looking at TTM | John Doe | Lois Lane on the apps account

The angular app gets at the last _ rows rather than the first rows. I'm going to have the React app start at the beginning so it is in alphabetical order. Does that sound good?

That is fine with me. The user prob won't be switching back and forth between both apps. If they were we might need to tell the user that we've changed the order, but I think we're good.

dr-bizz
dr-bizz reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! Thank you for making those styling changes.

The only thing left is that when you click confirm, it takes about 5 seconds for the request to return. We should make it more obvious to the user that it's working on the request in the background. Maybe adding a spinning wheel would help.

Also, after the user clicks confirm, we should prevent them from making more changes until the request is done.

@caleballdrin caleballdrin requested a review from dr-bizz June 20, 2024 00:57
dr-bizz
dr-bizz requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments that need to be addressed before merging into main.

Also, the confirm buttons were not sticking to the top of the page for me. I pulled your latest code. It might just be my local as it has been acting weird.

src/components/Tool/MergeContacts/MergeContacts.test.tsx Show resolved Hide resolved
src/components/GlobalStyles/GlobalStyles.tsx Outdated Show resolved Hide resolved
@caleballdrin caleballdrin requested a review from dr-bizz June 20, 2024 20:24
@dr-bizz
Copy link
Contributor

dr-bizz commented Jun 24, 2024

Looks great!

@caleballdrin caleballdrin merged commit 4b589bf into main Jun 24, 2024
18 checks passed
@caleballdrin caleballdrin deleted the 7987-merge-contacts branch June 24, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-bizz dr-bizz dr-bizz approved these changes

Assignees

@caleballdrin caleballdrin

Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caleballdrin @dr-bizz